Skip to content

Conversation

wawel37
Copy link
Contributor

@wawel37 wawel37 commented Oct 1, 2025

  1. We make it public as we need it in the cairo-lint and LS.
  2. We change the type to dyn Database, as we want to use this struct in LS, where we use the Database type from salsa already

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion


crates/cairo-lang-parser/src/db.rs line 53 at r1 (raw file):

/// Parses a file and returns the result and the generated [ParserDiagnostic].
#[salsa::tracked(returns(ref))]
pub fn file_syntax_data<'db>(db: &'db dyn Database, file_id: FileId<'db>) -> SyntaxData<'db> {

you don't need it - you have all the members of ParserGroup as the selectors for this data struct.

@wawel37 wawel37 force-pushed the file-syntax-public branch from f19356f to 7c9d8bf Compare October 2, 2025 11:45
Copy link
Contributor Author

@wawel37 wawel37 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @orizi)


crates/cairo-lang-parser/src/db.rs line 53 at r1 (raw file):

Previously, orizi wrote…

you don't need it - you have all the members of ParserGroup as the selectors for this data struct.

Ok, so cant we just make the syntax_file function part of the query group? As it's used in existing selectors anyway. And we don't want to duplicate logic of choosing right selector based on the FileId kind

@wawel37 wawel37 requested a review from orizi October 2, 2025 11:48
Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @wawel37)


crates/cairo-lang-parser/src/db.rs line 53 at r1 (raw file):

Previously, wawel37 (Mateusz Kowalski) wrote…

Ok, so cant we just make the syntax_file function part of the query group? As it's used in existing selectors anyway. And we don't want to duplicate logic of choosing right selector based on the FileId kind

i still don't see WHY you need it - what is the actual context?

Copy link
Contributor Author

@wawel37 wawel37 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @orizi)


crates/cairo-lang-parser/src/db.rs line 53 at r1 (raw file):

Previously, orizi wrote…

i still don't see WHY you need it - what is the actual context?

Why do we need to bother asserting in file_module_syntax, file_expr_syntax and file_statement_list_syntax for certain kinds, if we just want to get the SyntaxNode, no matter of the kind.
Here's this logic in lint: https://github.com/software-mansion/cairo-lint/blob/94a0ac6bf5bfeb7ce9308831f52aade3293fb9bd/src/upstream.rs#L8.
Here's same logic in LS: https://github.com/software-mansion/cairols/blob/8eb59905cf23be2fe2bf2d1a8a930411e9fa3933/src/lang/db/upstream.rs#L8.

Those 3 methods use the file_syntax beneath anyway, so why would we don't want to simplify it?

@wawel37 wawel37 requested a review from orizi October 6, 2025 12:11
Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

@orizi reviewed 1 of 2 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @wawel37)

Copy link
Collaborator

@piotmag769 piotmag769 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

@piotmag769 reviewed 1 of 1 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @wawel37)

@piotmag769 piotmag769 added this pull request to the merge queue Oct 6, 2025
Merged via the queue into starkware-libs:main with commit 12bc249 Oct 6, 2025
54 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants